Skip to content

Allow platform NaN canonicalization in floating-point assertions - #134316

Merged
tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-nan-payload-preservation
Sep 22, 2026
Merged

tannergooding merged 2 commits into
dotnet:mainfrom
tannergooding:tannergooding-nan-payload-preservation

Conversation

@tannergooding

Copy link
Copy Markdown
Member

WebAssembly floating-point operations can canonicalize NaNs rather than preserve the selected operand's payload and sign. Accept this platform behavior in the shared floating-point assertions instead of changing Min/Max lowering.

  • Centralize the test policy in PlatformDetection.IsNaNPayloadPreservationExpected, retaining the RISC-V exception and adding WebAssembly (browser and WASI).
  • Use the policy in the double, float, and Half assertions; NFloat delegates to the corresponding overload. Finite values and signed zeros remain bit-exact, and mismatches between NaN and non-NaN still fail.
  • Restore the four double Min/Max data rows previously excluded on Mono/Wasm. There are no product or JIT changes.

Validation: TestUtilities.csproj built successfully for net10.0 and net481. Browser-Wasm test execution was blocked by the baseline build failing with invalid-character errors in the extracted LLVM IRBuilder.h; no managed tests ran.

Resolves #134268

Note

This PR description was drafted with GitHub Copilot.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

@tannergooding

Copy link
Copy Markdown
Member Author

This is namely an issue in the V8 implementation here: https://github.com/v8/v8/blob/main/src/codegen/shared-ia32-x64/macro-assembler-shared-ia32-x64.cc#L290

They could do something more like what we do on xarch, which is notably also faster, but they don't today and explicitly canonicalize instead.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

This PR adjusts shared test assertions to tolerate WebAssembly NaN canonicalization while preserving exact checks for finite values and signed zero.

Changes:

  • Adds centralized NaN payload-preservation platform policy.
  • Applies it to double, float, and Half assertions.
  • Restores four WebAssembly Min/Max test cases.
File Description
src/​libraries/​Common/​tests/​TestUtilities/​System/​PlatformDetection.cs Updated as part of this pull request.
src/​libraries/​Common/​tests/​TestUtilities/​System/​AssertExtensions.cs Updated as part of this pull request.
src/​libraries/​Common/​tests/​System/​GenericMathTestMemberData.cs Updated as part of this pull request.

Comment thread src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 21, 2026 17:26
@tannergooding

Copy link
Copy Markdown
Member Author

Resolved merge conflicts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Low severity

Open (1)
Resolved since last review (1)

Comment thread src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs
@tannergooding
tannergooding merged commit d4b2951 into dotnet:main Sep 22, 2026
80 of 82 checks passed
@tannergooding
tannergooding deleted the tannergooding-nan-payload-preservation branch September 22, 2026 00:45
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 12.0-preview1 milestone Sep 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[browser][CoreCLR][R2R] Double Max/Min do not preserve selected NaN payload

3 participants